Skip to content

Rust: Remove source/library deduplication in path resolution #20192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 8, 2025

Removes logic that was left over from when library code was not extracted as source code.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Aug 8, 2025
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Aug 9, 2025
@hvitved hvitved marked this pull request as ready for review August 9, 2025 10:17
@hvitved hvitved requested a review from a team as a code owner August 9, 2025 10:17
@hvitved hvitved requested review from Copilot and geoffw0 August 9, 2025 10:17
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR removes legacy deduplication logic for source and library code in Rust path resolution. Previously, when both source and library versions of functions existed, the code preferred source versions, but this is no longer needed since library code is no longer extracted as source code.

  • Removes getASuccessorFull method and related deduplication logic
  • Renames getASuccessorFull to getASuccessor throughout the codebase
  • Updates all references to use the simplified successor method

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
rust/ql/lib/codeql/rust/internal/PathResolution.qll Removes deduplication logic and renames method calls from getASuccessorFull to getASuccessor
rust/ql/lib/codeql/rust/internal/CachedStages.qll Updates method reference in cached stages from getASuccessorFull to getASuccessor

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gather:

  • we used to extract library code less completely than code in the source.
  • the successors relation of interest here is on the item graph, it finds things that extend a path by a particular name.
  • we used to prefer taking the source successor, presumably because that would be more complete? than a successor defined in a library?
  • that's no longer necessary / useful because both versions would contain equally valid information now???

The last two bullets are guesswork. Please help fill the gaps in my understanding.

@hvitved
Copy link
Contributor Author

hvitved commented Aug 11, 2025

I gather:

  • we used to extract library code less completely than code in the source.
  • the successors relation of interest here is on the item graph, it finds things that extend a path by a particular name.
  • we used to prefer taking the source successor, presumably because that would be more complete? than a successor defined in a library?
  • that's no longer necessary / useful because both versions would contain equally valid information now???

The last two bullets are guesswork. Please help fill the gaps in my understanding.

Prior to #19506, library code was extracted in a way where a crate could be extracted both as source code and as library code, and we would have to do deduplication in QL. After that PR, everything gets extracted as source code, and the extractor does the deduplication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants